`Transport`: Bug fix in `rename` method #6735

ayushjariyal · 2025-01-28T18:33:36Z

Updated the condition to raise an appropriate error message (Destination already exists) when the path exists, improving code readability and correctness.

for more information, see https://pre-commit.ci

khsrali

Thanks @ayushjariyal for your contribution, please apply the review.

khsrali · 2025-01-29T09:20:14Z

src/aiida/transports/plugins/ssh.py

-            if not self.isdir(newpath):
-                raise OSError(f'Destination {newpath} does not exist')
+
+        if self.isfile(newpath) or self.isdir(newpath):


Suggested change

if self.isfile(newpath) or self.isdir(newpath):

if self.path_exists(newpath):

raise OSError(f'Destination {newpath} already exists')

maybe this is easier, and perhaps more readable.
Also I'm not entirely sure, if existing symlinks would get caught by self.isfile

Please also get rid of the comments above, line 1374-1377

for more information, see https://pre-commit.ci

ayushjariyal · 2025-01-29T10:02:10Z

@khsrali , I have made the changes as per your instructions. Could you please review them?

khsrali · 2025-01-29T10:08:18Z

@khsrali , I have made the changes as per your instructions. Could you please review them?

Please check here https://github.com/aiidateam/aiida-core/pull/6735/files
your files have merge conflicts. please fix that first.

ayushjariyal · 2025-01-29T11:18:50Z

@khsrali I solved the merge conflict. Can you please check it?

khsrali · 2025-01-29T15:39:39Z

bro, apply the review carefully 🥲
#6735 (comment)

danielhollas

I am quite confused by this bug. Does it mean that the function never worked at all?
Is it used anywhere else in the codebase? Would be good to have a test if possible.

danielhollas · 2025-01-29T21:02:24Z

src/aiida/transports/plugins/ssh.py

-                raise OSError(f'Destination {newpath} does not exist')
+
+        if self.path_exists(newpath):
+            raise OSError(f'Destination {newpath} already exist')


The docstring on line 1360 needs fixing. (github does not allow me to comment on that line)

In the docstring on line 1360, it states that OSError is used for both cases—when oldpath and newpath both are not found—which needs to be fixed. However, is it okay to use specific errors (FileNotFoundError and FileExistsError) instead of OSError?

:raises OSError: if oldpath is not found

In general. you are right FileNotFoundError might be a better fit for such cases, as with OSError is not immediately clear what's the error case.

However, please note OSError is the parent of FileNotFoundError

└── Exception └── OSError ├── FileNotFoundError ├── PermissionError ├── IsADirectoryError ├── NotADirectoryError

For the sake of consistency --I assume-- OSError was chose to raise from our Transport class with proper message. This way it's easier to handle, when adopting any of the Transport plugins.

khsrali · 2025-01-30T08:48:42Z

I am quite confused by this bug. Does it mean that the function never worked at all? Is it used anywhere else in the codebase? Would be good to have a test if possible.

yes, apparently never used across the interface 🥲

Agreed, adding tests is always a good idea! @ayushjariyal can you please write tests for this function? you can look for file test_all_plugins.py get inspiration from there and add your test with name def test_rename in it.
It's important before pushing you make sure it works, using pytest test_all_plugins.py::test_rename -s

ayushjariyal · 2025-02-06T05:52:12Z

@khsrali I implemented the following test_rename function

and it produce this result

Is it correct, or does it need any improvements? Can I proceed with pushing this function?

ayushjariyal · 2025-02-06T13:57:32Z

@danielhollas Previously, some test-generated files and directories were not properly deleted, causing warnings during pytest cleanup. I have now removed these leftover files, ensuring that no garbage files interfere with the test execution. The tests should now complete without OSError: Directory not empty warnings.

khsrali · 2025-02-06T14:02:30Z

@danielhollas Previously, some test-generated files and directories were not properly deleted, causing warnings during pytest cleanup. I have now removed these leftover files, ensuring that no garbage files interfere with the test execution. The tests should now complete without OSError: Directory not empty warnings.

@ayushjariyal thanks a lot! please push your changes so we can review it.
Don't worry about the garbage files, it's not related to your changes. That issue existed before.

danielhollas · 2025-02-06T14:04:29Z

Is it correct, or does it need any improvements? Can I proceed with pushing this function?

Looks reasonable. Always feel free to push, it's easier to discuss the code in GitHub UI once it is pushed. :-)

At the end of a test, if you expect that a call to a function should raise an exception, you should wrap it in pytest.raises(OSError) context manager, see https://docs.pytest.org/en/latest/how-to/assert.html#assertions-about-expected-exceptions

I think right now your test would pass even if the exception was not raised.

khsrali · 2025-02-06T14:17:25Z

@ayushjariyal can you also open an issue, documenting the problem with garbage files? Cheers!
So probably we could solve that later.

ayushjariyal · 2025-02-07T13:51:43Z

I encountered this issue while committing my code. Could you please clarify whether this error is due to something on my end or if it is system-dependent? I'm having trouble identifying the root cause.

khsrali · 2025-02-07T14:00:20Z

Doesn't look like issues imposed by your changes. It's more likely that mypy version you have installed doesn't match.
But don't worry, just commit & push. If you get block by pre-commit you can do pre-commit uninstall to remove the hooks.

Alternatively, before you commit, you can run git add . & pre-commit run.
And just check the issues from your changes, only. This is usually the way, I do it.

khsrali

Thanks a lot @ayushjariyal for the nice work! please see and apply the review.

khsrali · 2025-02-07T14:23:54Z

src/aiida/transports/plugins/local.py

+        # if not os.path.exists(newpath):
+        #     raise OSError(f'Destination {newpath} does not exist')
+
+        # Ensure the destination folder exists
+        dest_dir = os.path.dirname(newpath)
+        if dest_dir and not os.path.exists(dest_dir):
+            raise OSError(f'Destination directory {dest_dir} does not exist')


Thanks for noticing this! to me it's more convenient that you do the same check as you do for ssh.py.
I see that you suggest to check for directory, but in principle that should not be an issue, as this method should only be used for renaming and not moving files.

Suggested change

# if not os.path.exists(newpath):

# raise OSError(f'Destination {newpath} does not exist')

# Ensure the destination folder exists

dest_dir = os.path.dirname(newpath)

if dest_dir and not os.path.exists(dest_dir):

raise OSError(f'Destination directory {dest_dir} does not exist')

if os.path.exists(newpath):

raise OSError(f'Destination {newpath} already exists.')

also please update the docstring:

:raises OSError: if oldpath is not found or newpath already exists.

@ayushjariyal , maybe you forgot this? :)

khsrali · 2025-02-07T14:24:21Z

src/aiida/transports/plugins/ssh.py

@@ -1357,7 +1357,7 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath):
        :param str oldpath: existing name of the file or folder
        :param str newpath: new name for the file or folder

-        :raises OSError: if oldpath/newpath is not found
+        :raises OSError: if oldpath is not found


Suggested change

:raises OSError: if oldpath is not found

:raises OSError: if oldpath is not found or newpath already exists

khsrali · 2025-02-07T14:26:24Z

tests/transports/test_all_plugins.py

+        if not transport.is_open:
+            transport.open()


Suggested change

if not transport.is_open:

transport.open()

You don't need this, because the context manager already should take care of that. See here and here

khsrali · 2025-02-07T14:29:25Z

tests/transports/test_all_plugins.py

+            # Cleanup files manually
+            for file in tmp_path_remote.iterdir():
+                file.unlink()
+            tmp_path_remote.rmdir()  # Remove the directory if empty


Good that you think of clean up. But since you are using tmp_path_remote, it's not needed really. pytest should empty the directory automatically after the test returns.

Suggested change

# Cleanup files manually

for file in tmp_path_remote.iterdir():

file.unlink()

tmp_path_remote.rmdir() # Remove the directory if empty

khsrali · 2025-02-07T14:32:40Z

tests/transports/test_all_plugins.py

+        try:
+            transport.rename(new_file, another_file)
+            print('Rename to existing file succeeded (no error).')
+        except OSError as e:
+            print(f'Rename failed as expected: {e}')


pytest has a nice error capturing context manager, that can also match for an exact error message:

Suggested change

try:

transport.rename(new_file, another_file)

print('Rename to existing file succeeded (no error).')

except OSError as e:

print(f'Rename failed as expected: {e}')

with pytest.raises(OSError, match = "already exist"):

transport.rename(new_file, another_file)

for more information, see https://pre-commit.ci

codecov · 2025-02-09T19:33:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.14%. Comparing base (8c5c709) to head (383defc).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6735      +/-   ##
==========================================
+ Coverage   78.08%   78.14%   +0.06%     
==========================================
  Files         564      564              
  Lines       42547    42546       -1     
==========================================
+ Hits        33219    33242      +23     
+ Misses       9328     9304      -24

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

khsrali

@ayushjariyal , Thanks again for your contribution.
LGTM!

ayushjariyal · 2025-02-10T09:03:27Z

Thank you, @khsrali ! I really appreciate your time and review. Glad to contribute! Looking forward to more collaborations.

By the way, I was wondering if the organization plans to participate in GSoC 2025. I'd love to explore opportunities to contribute further.

khsrali · 2025-02-10T09:31:32Z

Thanks again :)

yes, you can check here https://github.com/aiidateam/aiida-core/wiki/GSoC-2025-Projects
We put some ideas there, but we don't have a slot yet. And the project is not approved, yet.

ayushjariyal · 2025-02-10T10:48:18Z

Thank you for sharing this, I really appreciate the information. I’ll check out the ideas and keep an eye on the updates. Looking forward to contributing more! 😊

ayushjariyal and others added 2 commits January 28, 2025 15:47

Solving bug in rename function

cfd8b31

[pre-commit.ci] auto fixes from pre-commit.com hooks

790204e

for more information, see https://pre-commit.ci

khsrali requested changes Jan 29, 2025

View reviewed changes

ayushjariyal and others added 2 commits January 29, 2025 15:23

Applying review on issue 6735

d4b2b4d

[pre-commit.ci] auto fixes from pre-commit.com hooks

a826a4c

for more information, see https://pre-commit.ci

ayushjariyal added 2 commits January 29, 2025 15:55

Resolved merge conflict in rename method

484a9f7

Solve merge conflicts

051ef84

applying review

2023dbe

danielhollas reviewed Jan 29, 2025

View reviewed changes

Update docstring

e2c34db

danielhollas self-requested a review February 6, 2025 12:36

Merge branch 'main' into issue_6725_rename_function_bug

abcc1b7

ayushjariyal added 2 commits February 7, 2025 19:43

Adding test_rename function

ba66b33

Adding test_rename function

6b6746e

khsrali requested changes Feb 7, 2025

View reviewed changes

ayushjariyal and others added 2 commits February 8, 2025 00:08

applying review

7fffa13

[pre-commit.ci] auto fixes from pre-commit.com hooks

778148d

for more information, see https://pre-commit.ci

ayushjariyal requested a review from khsrali February 9, 2025 07:05

ayushjariyal added 3 commits February 9, 2025 16:47

Applying review

b722475

applying review

cda46d0

fix test_rename function

35372ae

ayushjariyal force-pushed the issue_6725_rename_function_bug branch from 5ff694e to 35372ae Compare February 9, 2025 18:14

[pre-commit.ci] auto fixes from pre-commit.com hooks

383defc

for more information, see https://pre-commit.ci

khsrali approved these changes Feb 10, 2025

View reviewed changes

khsrali changed the title ~~Solving bug in rename function~~ Transport: Bug fix in rename method Feb 10, 2025

khsrali merged commit f56fcc3 into aiidateam:main Feb 10, 2025
10 checks passed

khsrali mentioned this pull request Feb 10, 2025

Transport: SshTransport bug in method rename #6725 solved #6750

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Transport`: Bug fix in `rename` method #6735

`Transport`: Bug fix in `rename` method #6735

ayushjariyal commented Jan 28, 2025

khsrali left a comment

khsrali Jan 29, 2025

khsrali Jan 29, 2025

ayushjariyal commented Jan 29, 2025

khsrali commented Jan 29, 2025

ayushjariyal commented Jan 29, 2025

khsrali commented Jan 29, 2025

danielhollas left a comment

danielhollas Jan 29, 2025

ayushjariyal Jan 30, 2025

khsrali Jan 30, 2025

khsrali Jan 30, 2025

khsrali commented Jan 30, 2025

ayushjariyal commented Feb 6, 2025

ayushjariyal commented Feb 6, 2025

khsrali commented Feb 6, 2025

danielhollas commented Feb 6, 2025

khsrali commented Feb 6, 2025

ayushjariyal commented Feb 7, 2025

khsrali commented Feb 7, 2025 •

edited

Loading

khsrali left a comment

khsrali Feb 7, 2025 •

edited

Loading

khsrali Feb 9, 2025

khsrali Feb 7, 2025

khsrali Feb 7, 2025

khsrali Feb 7, 2025

khsrali Feb 7, 2025 •

edited

Loading

codecov bot commented Feb 9, 2025 •

edited

Loading

khsrali left a comment

ayushjariyal commented Feb 10, 2025

khsrali commented Feb 10, 2025 •

edited

Loading

ayushjariyal commented Feb 10, 2025

	if self.isfile(newpath) or self.isdir(newpath):
	if self.path_exists(newpath):
	raise OSError(f'Destination {newpath} already exists')

	:raises OSError: if oldpath is not found
	:raises OSError: if oldpath is not found or newpath already exists

Transport: Bug fix in rename method #6735

Transport: Bug fix in rename method #6735

Conversation

ayushjariyal commented Jan 28, 2025

khsrali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayushjariyal commented Jan 29, 2025

khsrali commented Jan 29, 2025

ayushjariyal commented Jan 29, 2025

khsrali commented Jan 29, 2025

danielhollas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khsrali commented Jan 30, 2025

ayushjariyal commented Feb 6, 2025

ayushjariyal commented Feb 6, 2025

khsrali commented Feb 6, 2025

danielhollas commented Feb 6, 2025

khsrali commented Feb 6, 2025

ayushjariyal commented Feb 7, 2025

khsrali commented Feb 7, 2025 • edited Loading

khsrali left a comment

Choose a reason for hiding this comment

khsrali Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khsrali Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Feb 9, 2025 • edited Loading

Codecov Report

khsrali left a comment

Choose a reason for hiding this comment

ayushjariyal commented Feb 10, 2025

khsrali commented Feb 10, 2025 • edited Loading

ayushjariyal commented Feb 10, 2025

`Transport`: Bug fix in `rename` method #6735

`Transport`: Bug fix in `rename` method #6735

khsrali commented Feb 7, 2025 •

edited

Loading

khsrali Feb 7, 2025 •

edited

Loading

khsrali Feb 7, 2025 •

edited

Loading

codecov bot commented Feb 9, 2025 •

edited

Loading

khsrali commented Feb 10, 2025 •

edited

Loading